Vector.As<TFrom, TTo> #47150

huoyaoyuan · 2021-01-19T08:46:04Z

Closes #508

Not sure how to test it, because all the AsVectorXXX are not tested.

Questions:
Can we tread Unsafe.As to be reliably optimized by JIT and reduce the requirement for [Intrinsic]?
What's the simplest way to see the JIT and inlining decision on methods in CoreLib? jitdump or disasmo?

dotnet-issue-labeler · 2021-01-19T08:46:12Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-01-19T08:46:15Z

Tagging subscribers to this area: @tannergooding, @pgovind
See info in area-owners.md if you want to be subscribed.

Issue Details

Not sure how to test it, because all the AsVectorXXX are not tested.

Questions:
Can we tread Unsafe.As to be reliably optimized by JIT and reduce the requirement for [Intrinsic]?
What's the simplest way to see the JIT and inlining decision on methods in CoreLib? jitdump or disasmo?

Author:	huoyaoyuan
Assignees:	-
Labels:	`area-System.Numerics`, `new-api-needs-documentation`
Milestone:	-

gfoidl · 2021-01-19T10:57:56Z

JIT and inlining decision on methods in CoreLib? jitdump

IMHO yes, with COMPlus_JitDump set to the methodname -- you'll get a lot of (valuable) info, among them the JIT's reasoning for inlining (or not).

huoyaoyuan · 2021-01-19T11:47:00Z

I've tried JITDump but I remember I didn't get result for generic methods. (Or just generic for reference types)

SingleAccretion · 2021-01-19T12:07:01Z

Can we tread Unsafe.As to be reliably optimized by JIT and reduce the requirement for [Intrinsic]?

No, unfortunately not. Issues tend to crop up when you have things have their address taken in one way or another. This method will have to either be intrinsified (probably better in terms of Jit throughput and potentially CQ too) or use the existing AsXXX methods and a bunch of typeof(T) checks (no need to modify the native side).

What's the simplest way to see the JIT and inlining decision on methods in CoreLib? jitdump or disasmo?

For such simple methods that will be inlined unless you're out of budget, I'd use Disasmo (optionally with the dump option set). I'd write two methods that use Vector<T> and Ass somewhat heavily and compare the output between the existing methods and the generic one. The compilation problem should be solvable by decorating the method with aggressive inlining and then calling it from a non-generic wrapper.

tannergooding · 2021-01-19T16:24:29Z

src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs

+            ThrowHelper.ThrowForUnsupportedVectorBaseType<TFrom>();
+            ThrowHelper.ThrowForUnsupportedVectorBaseType<TTo>();
+
+            Debug.Assert(Vector<TFrom>.Count * Unsafe.SizeOf<TFrom>() == Vector<TTo>.Count * Unsafe.SizeOf<TTo>());


I don' think this assert is necessary. If the size, in bytes, of Vector<T> vs Vector<U> varies then we have much bigger problems in the JIT 😄

src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs

tannergooding · 2021-01-19T16:51:26Z

Could you update the top post with a comment similar to: "This resolves #508"?

Not sure how to test it, because all the AsVectorXXX are not tested.

I'd recommend just adding a few tests that validate converting T to U results in a bitwise equivalent value. You can always use CopyTo to get the raw bytes and then do a SequenceEqual comparison to validate correctness.
At a minimum, this should likely cover float <-> int as well as cases like int <-> ulong.
Likewise, a few test should be added validating that conversion to and from types like bool throws.

tannergooding · 2021-01-19T16:52:11Z

I think it was missed in API review, so I'll see if I can bring it up quickly today, but I think this should be an extension method like it is for Vector128<T> and Vector256<T>

tannergooding · 2021-01-21T21:07:32Z

Having this be an extension method was approved in API review.

tannergooding · 2021-01-24T16:09:01Z

Changes look good to me. It would just be good to get some basic tests covering a successful case and an error with an invalid type for TFrom and TTo

tannergooding · 2021-01-25T16:43:21Z

This still needs a test for the positive case, it should be mergeable after that is added.

huoyaoyuan · 2021-01-25T16:46:07Z

Does it need to evaluate the content of vector? Or just 0 is fine?

tannergooding · 2021-01-25T17:00:35Z

Does it need to evaluate the content of vector? Or just 0 is fine?

Ideally it would test it with an actual value to assert there aren't any bugs or accidental conversions (now or in the future) and that it is actually a bit-cast.
The test can always be something simple like:

var x = new Vector<int>(new int[] { 1, 2, 3, 4, 5, 6, 7, 8 });
var y = x.As<int, float>();

var xBytes = new byte[Vector<T>.Count * 4];
x.CopyTo(xBytes);

var yBytes = new byte[Vector<T>.Count * 4];
y.CopyTo(yBytes);

AssertEqual(xBytes, yBytes);

And repeat for a few cases. The ones that are likely most interesting to cover are same size to same size (e.g. float<->int and int<->uint) and same size to different size (e.g. double<->int and long<->int).

tannergooding · 2021-01-26T16:21:10Z

@echesakovMSFT, could you give the HWIntrinsic changes a quick secondary review?

echesakov

LGTM

src/libraries/System.Numerics.Vectors/tests/GenericVectorUnsupportedTests.cs

tannergooding · 2021-02-01T16:06:28Z

@huoyaoyuan, I'm investigating the alpine failure.

I'm also going to quickly close and reopen the PR so the CI jobs pick up latest master and therefore the System.Linq.Expressions test fix

tannergooding · 2021-02-02T17:39:23Z

src/coreclr/jit/simdashwintrinsic.cpp

@@ -405,6 +405,22 @@ GenTree* Compiler::impSimdAsHWIntrinsicSpecial(NamedIntrinsic       intrinsic,
        }
 #endif // TARGET_X86

+#if defined(TARGET_XARCH)


The issue is that this is actually under a #if defined(TARGET_XARCH) already (defined on L385) and so this is never hit on arm64.

The simplest fix is to remove the #if defined(TARGET_XARCH) here and then copy the following logic into the #elif defined(TARGET_ARM64) on L446

switch (intrinsic) { case NI_VectorT128_As: { unsigned retSimdSize; var_types retBaseType = getBaseTypeAndSizeOfSIMDType(sig->retTypeSigClass, &retSimdSize); if (!varTypeIsArithmetic(retBaseType) || (retSimdSize == 0)) { // We get here if the return type is an unsupported type return nullptr; } break; } default: { // Most intrinsics have some path that works even if only AdvSimd is available break; } }

Got it. I'll look at the structure and find the opportunity to share code.

I reordered the ifdef block into two. One for general instruction set availability, one for special logic for each instruction. Is this sure?

It looks correct. I think this will be good to merge if CI passes

tannergooding · 2021-02-03T15:21:45Z

Thanks for the PR @huoyaoyuan!

huoyaoyuan added 2 commits January 19, 2021 16:30

Implement Vector.As using Unsafe.As.

482d937

Ref source for Vector.As.

f943af0

dotnet-issue-labeler bot added area-System.Numerics new-api-needs-documentation labels Jan 19, 2021

tannergooding reviewed Jan 19, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs Outdated Show resolved Hide resolved

huoyaoyuan added 4 commits January 20, 2021 15:29

Mark managed impl with intrinsic and aggressive inlining.

0ea40f7

Hwintrinsic definition

7a8d51d

Hwintrinsic impl

58e2ff5

Add missing break.

52e6b10

huoyaoyuan force-pushed the vector-as branch from d534299 to 52e6b10 Compare January 20, 2021 14:21

runfoapp bot mentioned this pull request Jan 20, 2021

Inability to unzip assets during build on Unix x64 #32805

Closed

Make extension method.

6ebb89d

huoyaoyuan requested a review from tannergooding January 24, 2021 07:53

Add test for unsupported As types.

13c979c

runfoapp bot mentioned this pull request Jan 25, 2021

Testing out the new system jaredpar/runfo#71

Open

Add test for op_explicit and cast.

a6e021b

tannergooding approved these changes Jan 26, 2021

View reviewed changes

runfoapp bot mentioned this pull request Jan 26, 2021

System.Linq.Expressions tests failing on some PRs #47374

Closed

echesakov approved these changes Jan 26, 2021

View reviewed changes

tannergooding reviewed Jan 26, 2021

View reviewed changes

src/libraries/System.Numerics.Vectors/tests/GenericVectorUnsupportedTests.cs Show resolved Hide resolved

tannergooding closed this Feb 1, 2021

tannergooding reopened this Feb 1, 2021

tannergooding reviewed Feb 2, 2021

View reviewed changes

Reorder intrinsic checks.

8f3078f

huoyaoyuan closed this Feb 3, 2021

huoyaoyuan reopened this Feb 3, 2021

tannergooding merged commit fabaed2 into dotnet:master Feb 3, 2021

huoyaoyuan deleted the vector-as branch February 3, 2021 15:28

ghost locked as resolved and limited conversation to collaborators Mar 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vector.As<TFrom, TTo> #47150

Vector.As<TFrom, TTo> #47150

huoyaoyuan commented Jan 19, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Jan 19, 2021

ghost commented Jan 19, 2021

gfoidl commented Jan 19, 2021

huoyaoyuan commented Jan 19, 2021

SingleAccretion commented Jan 19, 2021 •

edited

Loading

tannergooding Jan 19, 2021

tannergooding commented Jan 19, 2021

tannergooding commented Jan 19, 2021

tannergooding commented Jan 21, 2021

tannergooding commented Jan 24, 2021

tannergooding commented Jan 25, 2021

huoyaoyuan commented Jan 25, 2021

tannergooding commented Jan 25, 2021

tannergooding commented Jan 26, 2021

echesakov left a comment

tannergooding commented Feb 1, 2021

tannergooding Feb 2, 2021 •

edited

Loading

huoyaoyuan Feb 2, 2021

huoyaoyuan Feb 2, 2021

tannergooding Feb 2, 2021

tannergooding commented Feb 3, 2021

Vector.As<TFrom, TTo> #47150

Vector.As<TFrom, TTo> #47150

Conversation

huoyaoyuan commented Jan 19, 2021 • edited Loading

dotnet-issue-labeler bot commented Jan 19, 2021

ghost commented Jan 19, 2021

gfoidl commented Jan 19, 2021

huoyaoyuan commented Jan 19, 2021

SingleAccretion commented Jan 19, 2021 • edited Loading

tannergooding Jan 19, 2021

Choose a reason for hiding this comment

tannergooding commented Jan 19, 2021

tannergooding commented Jan 19, 2021

tannergooding commented Jan 21, 2021

tannergooding commented Jan 24, 2021

tannergooding commented Jan 25, 2021

huoyaoyuan commented Jan 25, 2021

tannergooding commented Jan 25, 2021

tannergooding commented Jan 26, 2021

echesakov left a comment

Choose a reason for hiding this comment

tannergooding commented Feb 1, 2021

tannergooding Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

huoyaoyuan Feb 2, 2021

Choose a reason for hiding this comment

huoyaoyuan Feb 2, 2021

Choose a reason for hiding this comment

tannergooding Feb 2, 2021

Choose a reason for hiding this comment

tannergooding commented Feb 3, 2021

huoyaoyuan commented Jan 19, 2021 •

edited

Loading

SingleAccretion commented Jan 19, 2021 •

edited

Loading

tannergooding Feb 2, 2021 •

edited

Loading